Skip to content

fix: pre-public audit cleanup#2

Merged
newtsjamie merged 3 commits into
mainfrom
fix/pre-public-audit-cleanup
Mar 29, 2026
Merged

fix: pre-public audit cleanup#2
newtsjamie merged 3 commits into
mainfrom
fix/pre-public-audit-cleanup

Conversation

@newtsjamie

@newtsjamie newtsjamie commented Mar 29, 2026

Copy link
Copy Markdown
Member

Summary

Audit cleanup before making the circuits repo public.

  • Remove dead src/ directory — obsolete circuit versions with weaker 2-limb RSA hashing, superseded by proof_a/proof_b packages. Could confuse reviewers and be mistaken for production code.
  • Fix location sign verification in selective_disclosuredisclosed_location_flags sign bits were described in comments but never actually asserted against exact coordinate signs. An attacker could theoretically claim a southern-hemisphere coordinate is in a northern bounding box.
  • Add missing LICENSE for vendored noir-rsa — MIT license from upstream zkpassport repo, plus VENDORED_FROM.md attribution.
  • Remove vendor/noir-jwt/js/ — contained test RSA private key and npm build artifacts not needed for circuit compilation.
  • Update README versions — nargo beta.18 to beta.19, Aztec v3.0.3 to v4 devnet, dependency versions to match current Nargo.toml files.
  • Fix Prover.toml — update to use location_flags field instead of removed center_lat_is_negative/center_lon_is_negative fields.

Test plan

  • CI passes (all circuits compile)
  • Selective disclosure circuit compiles with new sign asserts
  • No secrets, credentials, or sensitive data remaining in repo
  • All vendored code has proper LICENSE and attribution

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Documentation

    • Updated Noir toolchain to v1.0.0-beta.19 and Aztec target to v4 devnet; revised binding-generation guidance (no pinned builder version); bumped several documented dependency versions; added MIT license/vendor note for noir_rsa; clarified webauthn session expiry handling.
  • Refactor

    • Consolidated location coordinate sign bits into a single unified flag and updated related validation semantics.
  • Chores

    • Removed multiple JWT/certificate proof circuits, associated tests, and vendor JS packaging/tools.

- Remove dead src/ directory (obsolete circuit versions with weaker
  2-limb RSA hashing, superseded by proof_a/proof_b packages)
- Add missing location sign verification asserts in selective_disclosure
  (disclosed lat/lon signs must match exact coordinate signs)
- Add LICENSE and VENDORED_FROM.md for vendored noir-rsa
- Remove vendor/noir-jwt/js/ (test RSA private key, npm artifacts
  not needed for circuit compilation)
- Update README: nargo beta.18 -> beta.19, Aztec v3 -> v4 devnet,
  dependency versions to match current Nargo.toml files
- Fix Prover.toml: use location_flags field instead of removed
  center_lat_is_negative/center_lon_is_negative fields

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Mar 29, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ed92651e-fd36-47d9-9047-bb5967730035

📥 Commits

Reviewing files that changed from the base of the PR and between 5affc9a and 1d5c563.

📒 Files selected for processing (1)
  • selective_disclosure/src/main.nr
🚧 Files skipped from review as they are similar to previous changes (1)
  • selective_disclosure/src/main.nr

📝 Walkthrough

Walkthrough

Removes three Noir circuit files, deletes the noir-jwt vendor JS/TS tooling, consolidates two location sign-bit public inputs into one location_flags field, updates README dependency/version/Aztec target text, and adds vendored noir-rsa license and provenance notes.

Changes

Cohort / File(s) Summary
Documentation & Version Updates
README.md
Updated Noir toolchain requirement to nargo v1.0.0-beta.19, Aztec target to v4 devnet, removed pinned @aztec/builder version, and bumped several documented dependency versions and notes.
Location Flag Refactor
proof_a/Prover.toml, selective_disclosure/src/main.nr
Replaced center_lat_is_negative/center_lon_is_negative with single location_flags bitfield; updated comments, toml value, and circuit logic to extract/assert latitude/longitude sign bits.
Removed Noir Circuits
src/main.nr, src/proof_a.nr, src/proof_b.nr
Deleted three full Noir circuit implementations, their constants, helpers, and unit tests (certificate/COSE verification, Merkle/link/nullifier logic, timestamp bounds).
Removed noir-jwt Vendor Tools
vendor/noir-jwt/js/package.json, vendor/noir-jwt/js/scripts/noir-test-data.ts, vendor/noir-jwt/js/src/generate-inputs.ts, vendor/noir-jwt/js/src/partial-sha.ts, vendor/noir-jwt/js/src/index.ts, vendor/noir-jwt/js/tsconfig.json
Removed package manifest, TS config, input-generation and partial-SHA utilities, test-data script, and index re-exports—eliminates JWT→circuit input generation tooling.
Vendored noir-rsa Provenance
vendor/noir-rsa/LICENSE, vendor/noir-rsa/VENDORED_FROM.md
Added MIT LICENSE and VENDORED_FROM.md documenting source/version and a sha512 bump for compatibility.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐇 I nibble lines of code at dawn,

Two flags in one now carry on,
Old JWT trails slipped from the lawn,
noir‑rsa roots in new light drawn,
A hopping cheer — the patches gone.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title "fix: pre-public audit cleanup" directly matches the PR's primary objective—removing dead code, fixing issues, and preparing the circuits repository for public release before an audit.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/pre-public-audit-cleanup

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
vendor/noir-rsa/VENDORED_FROM.md (1)

3-6: Add upstream commit SHA for immutable vendoring provenance.

Source + tag is good, but audits are more reproducible if this file also records the exact upstream commit hash that was vendored.

Suggested doc update
 # Vendored: noir_rsa

 - **Source**: https://github.com/zkpassport/noir_rsa
 - **Version**: v0.9.1 (with sha512 bumped to v0.1.1 for beta.19 compatibility)
+- **Upstream commit**: <full_commit_sha>
 - **License**: MIT (https://github.com/zkpassport/noir_rsa/blob/main/LICENSE)
 - **Authors**: [zkpassport](https://zkpassport.id/)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@vendor/noir-rsa/VENDORED_FROM.md` around lines 3 - 6, The VENDORED_FROM.md
entry for noir-rsa currently lists Source, Version, License, and Authors but
lacks the exact upstream commit SHA; update the file by adding an explicit
"Commit" (or "Upstream commit") field containing the full commit hash
corresponding to the vendored code (alongside the existing Source and Version
lines) so audits are reproducible; ensure the new line follows the same markdown
bullet style and references the exact commit for the tag/version noted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@README.md`:
- Around line 134-140: Update the README entry for noir_rsa to explicitly state
that the dependency table lists the active circuit/runtime dependency versions
(e.g., noir_rsa 0.10.0) and that vendored snapshots may be independently pinned
and differ (e.g., the vendored noir_rsa v0.9.1 noted in VENDORED_FROM.md); add a
short parenthetical or footnote near the noir_rsa row clarifying "table = active
dependencies; vendored copies may show older pinned versions (see
VENDORED_FROM.md)".

In `@selective_disclosure/src/main.nr`:
- Around line 223-228: Cast disclosed_location_flags from Field to a small
unsigned integer (e.g., u8) and constrain it to 2 bits before doing bitwise ops:
convert disclosed_location_flags to an integer variable (flags_u8), assert
flags_u8 < 4 to constrain to 2 bits, compute disclosed_lat_neg = flags_u8 & 1
and disclosed_lon_neg = (flags_u8 >> 1) & 1, then compare those with
exact_lat_is_negative and exact_lon_is_negative via the existing assert calls;
use the same variable names (disclosed_location_flags, disclosed_lat_neg,
disclosed_lon_neg, exact_lat_is_negative, exact_lon_is_negative) so the logic is
identical but uses integer bitwise operators.

---

Nitpick comments:
In `@vendor/noir-rsa/VENDORED_FROM.md`:
- Around line 3-6: The VENDORED_FROM.md entry for noir-rsa currently lists
Source, Version, License, and Authors but lacks the exact upstream commit SHA;
update the file by adding an explicit "Commit" (or "Upstream commit") field
containing the full commit hash corresponding to the vendored code (alongside
the existing Source and Version lines) so audits are reproducible; ensure the
new line follows the same markdown bullet style and references the exact commit
for the tag/version noted.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0a38bc1f-f935-40f8-9a37-8a1668e90706

📥 Commits

Reviewing files that changed from the base of the PR and between 7ed1163 and 7bb01ce.

⛔ Files ignored due to path filters (1)
  • vendor/noir-jwt/js/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (14)
  • README.md
  • proof_a/Prover.toml
  • selective_disclosure/src/main.nr
  • src/main.nr
  • src/proof_a.nr
  • src/proof_b.nr
  • vendor/noir-jwt/js/package.json
  • vendor/noir-jwt/js/scripts/noir-test-data.ts
  • vendor/noir-jwt/js/src/generate-inputs.ts
  • vendor/noir-jwt/js/src/index.ts
  • vendor/noir-jwt/js/src/partial-sha.ts
  • vendor/noir-jwt/js/tsconfig.json
  • vendor/noir-rsa/LICENSE
  • vendor/noir-rsa/VENDORED_FROM.md
💤 Files with no reviewable changes (9)
  • vendor/noir-jwt/js/src/index.ts
  • vendor/noir-jwt/js/tsconfig.json
  • vendor/noir-jwt/js/package.json
  • vendor/noir-jwt/js/src/partial-sha.ts
  • vendor/noir-jwt/js/scripts/noir-test-data.ts
  • vendor/noir-jwt/js/src/generate-inputs.ts
  • src/proof_a.nr
  • src/proof_b.nr
  • src/main.nr

Comment thread README.md Outdated
Comment thread selective_disclosure/src/main.nr
newtsjamie and others added 2 commits March 29, 2026 16:17
…rification

Noir does not support bitwise operators on Field types. Cast to u8
with a <= 3 constraint before extracting sign bits. Also clarify
vendored noir_rsa version discrepancy in README.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bit0=lat_neg=0, bit1=lon_neg=1 → flags=2 (was 1, which asserted lat_neg=1 incorrectly)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@newtsjamie newtsjamie merged commit 5af56f8 into main Mar 29, 2026
18 checks passed
@newtsjamie newtsjamie deleted the fix/pre-public-audit-cleanup branch March 29, 2026 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant